src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)

85-120: ⚠️ Potential issue | 🟠 Major

Upload-then-persist ordering can orphan S3 objects.

When the multipart upload succeeds (Line 102) but visaService.applyForVisa(...) later fails (e.g., DB constraint, travel-date race), the uploaded object remains in S3 with no DB reference. Same pattern in processUpdate (Lines 175–179). The root-cause note and a suggested fix are in the VisaService review; from the controller's side, consider delegating the upload into the service so both operations live under a single transactional/compensation boundary, or at minimum call fileService.deleteFile(s3Key) in the catch blocks before returning the form.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`
around lines 85 - 120, The controller currently uploads to S3 before calling
visaService.applyForVisa in submitApplication (and similarly in processUpdate),
which can leave orphaned S3 objects if the DB operation fails; either move the
upload logic into visaService.applyForVisa/processUpdate so the service can
manage the upload within its transactional/compensation boundary, or if you keep
upload in the controller, ensure any uploaded s3Key is cleaned up on failure by
calling fileService.deleteFile(s3Key) (guarded by s3Key != null) in each catch
block (IOException, IllegalArgumentException and any other failure paths) before
returning the form; update submitApplication and processUpdate accordingly so S3
cleanup always happens on error.

src/test/java/org/example/visacasemanagementsystem/visa/VisaMapperTest.java (1)

80-101: Consider adding a test for the new statusInformation mapping behavior.

The mapper now conditionally sets statusInformation only when non-null (VisaMapper.java:59-61). This test only exercises the null branch, leaving the non-null case uncovered. Worth adding an assertion that (a) a non-null statusInformation in the DTO is written through, and (b) a null value does not overwrite a pre-existing value on existingVisa.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaMapperTest.java`
around lines 80 - 101, Add two assertions and a new test case exercising
VisaMapper.updateEntityFromDTO's statusInformation branch: create an
UpdateVisaDTO with a non-null statusInformation and assert
existingVisa.getStatusInformation() is updated; also assert in the current test
(or a new test) that when DTO.statusInformation() is null an existing non-null
existingVisa.statusInformation remains unchanged. Reference the UpdateVisaDTO
construction and the visaMapper.updateEntityFromDTO(...) call and assert against
existingVisa.getStatusInformation() to cover both non-null and null branches.

src/main/resources/application.properties (1)

21-22: Consider aligning multipart limits with S3/Tomcat expectations.

10MB is reasonable for passport scans, but VisaViewController accepts the upload synchronously and streams through the service. If you ever accept larger documents, also tune server.tomcat.max-swallow-size and server.tomcat.max-http-form-post-size, otherwise requests exceeding those (defaults 2MB/2MB respectively in some configurations) will fail before Spring enforces the multipart limit. Not a blocker at 10MB.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/application.properties` around lines 21 - 22, The
multipart size properties spring.servlet.multipart.max-file-size and
spring.servlet.multipart.max-request-size are set to 10MB but Tomcat can reject
larger uploads first; update configuration to also set
server.tomcat.max-swallow-size and server.tomcat.max-http-form-post-size to
values equal to or larger than the multipart limits (or to -1 for unlimited) so
uploads accepted by VisaViewController’s synchronous streaming won’t be dropped
by Tomcat before Spring processes them; ensure the same limit is applied to both
max-file-size and max-request-size and document the chosen values.

compose.yaml (1)

11-24: Local-dev credentials and healthcheck.

Two small improvements for the MinIO service:

1. Parameterize credentials via .env so they aren't duplicated between compose.yaml and application.properties, and so prod-ish environments don't silently reuse admin/password.
2. Add a healthcheck so dependent services (app container, future init jobs) can wait for MinIO to be ready before FileService's bucket-init runs. mc ready local or an HTTP probe on /minio/health/ready both work.

♻️ Suggested change

   minio:
     image: 'minio/minio:latest'
     ports:
       - '9000:9000'
       - '9001:9001'
     environment:
-      - MINIO_ROOT_USER=admin
-      - MINIO_ROOT_PASSWORD=password
+      - MINIO_ROOT_USER=${MINIO_ROOT_USER:-admin}
+      - MINIO_ROOT_PASSWORD=${MINIO_ROOT_PASSWORD:-password}
     command: server /data --console-address ":9001"
     volumes:
       - minio_data:/data
+    healthcheck:
+      test: ["CMD", "curl", "-f", "http://localhost:9000/minio/health/ready"]
+      interval: 10s
+      timeout: 5s
+      retries: 5

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@compose.yaml` around lines 11 - 24, The MinIO service currently hardcodes
credentials and lacks a readiness probe; change the minio service to read
MINIO_ROOT_USER and MINIO_ROOT_PASSWORD from environment variables (use
${MINIO_ROOT_USER} / ${MINIO_ROOT_PASSWORD} with sensible defaults via .env)
instead of fixed "admin"/"password" so they can be shared with
application.properties, and add a healthcheck block for the minio service (e.g.,
using "healthcheck" with an HTTP probe to /minio/health/ready or an sh -c 'mc
alias set ... && mc admin info local' / 'mc ready local' command) so dependent
services and FileService bucket-init wait until MinIO is ready; update .env with
the variables and ensure application.properties references the same env vars.

pom.xml (1)

156-160: Prefer AWS SDK BOM over pinning a single artifact version.

Importing software.amazon.awssdk:bom via <dependencyManagement> keeps all AWS SDK modules (core, regions, auth, http-client, s3, etc.) aligned on one version and avoids subtle transitive-version mismatches as you add more SDK modules. Additionally, version 2.42.30 is now the latest stable release; update from 2.42.20.

♻️ Suggested structure

+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>software.amazon.awssdk</groupId>
+                <artifactId>bom</artifactId>
+                <version>2.42.30</version>
+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>
...
         <dependency>
             <groupId>software.amazon.awssdk</groupId>
             <artifactId>s3</artifactId>
-            <version>2.42.20</version>
         </dependency>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 156 - 160, Replace the hard-pinned
software.amazon.awssdk:s3 dependency version with a BOM-based approach: add
software.amazon.awssdk:bom at version 2.42.30 under dependencyManagement (import
scope) and remove the <version> element from the existing
<artifactId>s3</artifactId> dependency so the S3 module inherits the BOM-managed
version; ensure any other AWS SDK modules follow the same pattern so all SDK
modules (core, regions, auth, http-client, s3, etc.) remain aligned.

src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)

129-148: Stub and verify the S3 key flow in the upload tests.

The tests at lines 129–148 and 224–245 attach passportFile but use any() for the S3 key parameter, allowing the controller to skip FileService.uploadFile() entirely (or pass null) and still pass. Stub fileService.uploadFile() to return a specific key, verify the upload was called, and assert that exact key reaches VisaService.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`
around lines 129 - 148, The test currently uses any() for the S3 key allowing
the controller to skip or ignore FileService.uploadFile(); update the test to
stub fileService.uploadFile(...) to return a fixed S3 key (e.g.
"s3://bucket/key.pdf"), assert fileService.uploadFile was called with the
MockMultipartFile (passportFile), and change the verify of
visaService.applyForVisa(...) to assert that the exact returned S3 key is passed
into the CreateVisaDTO (or passed as the third parameter) instead of any();
locate MockMultipartFile creation, the mockMvc.perform multipart call, and the
verify(visaService).applyForVisa(...) in this test and the similar block at
lines ~224–245 to add the when(...).thenReturn(...) for fileService.uploadFile,
a verify(fileService).uploadFile(...) call, and a stricter verification of the
S3 key.

src/test/java/org/example/visacasemanagementsystem/TestcontainersConfiguration.java (1)

15-18: Pin the PostgreSQL image instead of latest.

postgres:latest is mutable; CI can silently move to a new major version and make Testcontainers runs non-reproducible. Pin the PostgreSQL major/minor version that matches production.

♻️ Example pin, if PostgreSQL 16 matches the supported runtime

-        return new PostgreSQLContainer<>(DockerImageName.parse("postgres:latest"))
+        return new PostgreSQLContainer<>(DockerImageName.parse("postgres:16-alpine"))
                 .withDatabaseName("testdb")
                 .withUsername("test")
                 .withPassword("test");

Run this read-only check after updating to ensure mutable tags are gone:

#!/bin/bash
# Description: Find mutable postgres image tags in test configuration.
rg -n -C2 'postgres:latest|DockerImageName\.parse\("postgres:' 

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/TestcontainersConfiguration.java`
around lines 15 - 18, The Testcontainers configuration is using a mutable image
tag ("postgres:latest") in the PostgreSQLContainer creation inside
TestcontainersConfiguration; replace DockerImageName.parse("postgres:latest")
with a pinned major.minor production-matching tag (e.g., "postgres:16.2" or the
exact version you run in prod) or load that pinned tag from a test
config/property, keeping the same PostgreSQLContainer construction and
.withDatabaseName/.withUsername/.withPassword calls; after updating, run the
provided grep/ripgrep check to ensure no remaining mutable postgres tags remain.

src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java (1)

61-89: findVisaDtoById rebuilds the entire VisaDTO just to inject presigned URLs.

This manual re-enumeration of every field is fragile: any new field added to VisaDTO must be remembered here or the detail view silently loses it. It also duplicates responsibility with VisaMapper.toDTO (which sets downloadUrls to List.of() for the exact reason that this method will replace them).

A cleaner approach is to push the "enrich with presigned URLs" step into a small helper (or a VisaMapper.toDtoWithUrls(visa, fileService) overload), so only one code path constructs VisaDTO end-to-end.

♻️ Proposed fix sketch

-    public VisaDTO findVisaDtoById(Long id) {
-        Visa visa = visaRepository.findById(id)
-                .orElseThrow(() -> new EntityNotFoundException(NOT_FOUND_MESSAGE));
-
-        VisaDTO dto = visaMapper.toDTO(visa);
-
-        List<String> presignedUrls = visa.getS3Keys().stream()
-                .map(fileService::getPresignedDownloadUrl)
-                .toList();
-
-        return new VisaDTO(
-                dto.id(), dto.visaType(), dto.visaStatus(), dto.nationality(),
-                dto.passportNumber(), dto.travelDate(), dto.applicantId(),
-                dto.applicantName(), dto.handlerId(), dto.handlerName(),
-                dto.createdAt(), dto.updatedAt(), dto.statusInformation(),
-                presignedUrls, dto.s3Keys()
-        );
-    }
+    public VisaDTO findVisaDtoById(Long id) {
+        Visa visa = visaRepository.findById(id)
+                .orElseThrow(() -> new EntityNotFoundException(NOT_FOUND_MESSAGE));
+        return visaMapper.toDtoWithDownloadUrls(visa, fileService::getPresignedDownloadUrl);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java`
around lines 61 - 89, The findVisaDtoById method manually rebuilds VisaDTO to
set presignedUrls, which is fragile and duplicates VisaMapper.toDTO; instead add
a single helper such as VisaMapper.toDtoWithUrls(Visa visa, FileService
fileService) (or a small private method mapWithUrls) that calls toDTO(visa) and
replaces/downloads the presigned URLs by mapping visa.getS3Keys() via
fileService.getPresignedDownloadUrl, then return that DTO from findVisaDtoById;
update findVisaDtoById to call the new mapper/helper and remove the manual
reconstruction so future VisaDTO fields don’t need to be mirrored here.

src/main/java/org/example/visacasemanagementsystem/config/S3Config.java (2)

18-28: Consider @ConfigurationProperties and secret-hardening for S3 credentials.

Four individual @Value injections duplicated across two beans work, but a single @ConfigurationProperties("minio") record/class would centralize the config and make it easier to validate (e.g., @NotBlank on endpoint, accessKey, secretKey). Also flag: plain accessKey/secretKey coming from properties are fine for local MinIO, but for any non-dev deployment these should come from an env/secret store (SSM, Vault, K8s secrets) rather than be bundled into application.properties.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/config/S3Config.java`
around lines 18 - 28, Replace the scattered `@Value` injections in S3Config with a
single `@ConfigurationProperties`(prefix="minio")-annotated class or record (e.g.,
MinioProperties) that declares endpoint, accessKey, secretKey, region with
validation annotations like `@NotBlank`; inject that MinioProperties into S3Config
and the other bean instead of duplicating fields, add `@Validated` on the
configuration class to enforce constraints, and update documentation/config so
accessKey/secretKey are sourced from environment/secret-management (SSM, Vault,
or K8s secrets) for non-dev deployments rather than stored in
application.properties.

---

38-42: Remove stray commented-out code.

The checksumValidationEnabled(false) line on Line 40 is commented out. If it's not needed, remove it to keep the configuration clean; if it was intended to be enabled to work around MinIO's handling of AWS SDK v2 default integrity checks, make it explicit rather than leaving it as a dead artifact.

♻️ Suggested cleanup

                 .serviceConfiguration(S3Configuration.builder()
                         .pathStyleAccessEnabled(true)
-//                        .checksumValidationEnabled(false)
                         .build())

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/config/S3Config.java`
around lines 38 - 42, In S3Config update the S3Configuration builder call:
either delete the commented-out ".checksumValidationEnabled(false)" line to
remove dead code, or make the intent explicit by uncommenting and setting
checksumValidationEnabled(false) on the S3Configuration builder
(S3Configuration.builder() ... .checksumValidationEnabled(false) ... .build())
if you need to disable SDK integrity checks for MinIO; ensure the change is
applied where S3Configuration is constructed inside the S3Config class.

src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (2)

189-203: Brittle message-string branching for IllegalArgumentException.

e.getMessage().contains("date") is a fragile way to distinguish travel-date errors from other IllegalArgumentExceptions thrown by the service (e.g., "Mismatched visa id.", "This application can no longer be edited."). Any wording change in VisaService silently flips the error from a field error on travelDate to a global error (or vice versa). Prefer a dedicated exception type (e.g., InvalidTravelDateException) or return codes, and branch on the type rather than the message text.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`
around lines 189 - 203, The current catch in VisaViewController relies on
inspecting IllegalArgumentException.getMessage() to detect travel-date errors
(e.getMessage().contains("date")), which is brittle; update the service and
controller to use a dedicated exception type instead: add an
InvalidTravelDateException (or similar) thrown by VisaService when the travel
date is invalid, then in the try/catch replace the message-inspection branch
with a catch(InvalidTravelDateException e) that calls
bindingResult.rejectValue("travelDate", "error.travelDate", e.getMessage());
keep a separate catch(IllegalArgumentException e) to handle other
illegal-argument cases and call bindingResult.reject("globalError",
e.getMessage()); ensure prepareApplyModel(principal.getUserId(), model),
visaService.findVisaDtoById(id) and the model attributes remain as-is in both
branches.

---

208-216: Document-delete endpoint has no user-visible error feedback on failure.

visaService.removeVisaDocument(...) can throw UnauthorizedException or silently no-op if the key isn't attached. The controller simply redirects to /visas/{id}/edit either way; the user sees no confirmation on success nor an error on failure. Consider using RedirectAttributes.addFlashAttribute(...) to surface both outcomes on the edit page. Also worth wrapping in a try/catch around EntityNotFoundException so a bad s3Key returns a friendly message instead of a 500 page via the global exception handler.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`
around lines 208 - 216, Update the deleteVisaDocument controller to catch and
surface outcomes to the user: wrap the call to
visaService.removeVisaDocument(id, s3Key, principal.getUserId()) in a try/catch
that handles UnauthorizedException and EntityNotFoundException (and a generic
Exception fallback), add a RedirectAttributes parameter to deleteVisaDocument,
and use redirectAttributes.addFlashAttribute(...) to add a success message when
removal succeeds or an error message when exceptions occur before returning
"redirect:/visas/" + id + "/edit"; ensure UnauthorizedException produces a clear
permission error and EntityNotFoundException produces a friendly "document not
found" message rather than letting the global handler return 500.

src/main/java/org/example/visacasemanagementsystem/file/FileService.java (2)

89-102: responseContentDisposition uses the raw S3 key as the filename.

The key is UUID + "_" + original, so users downloading always see the UUID prefix in their file name, and if original ever contained a " or newline (upstream sanitization would help, see the uploadFile comment), it would break the Content-Disposition header. Consider storing the original filename as S3 object metadata at upload time and reading it back here (RFC 5987-encoded) so downloads present the original, safe filename.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java`
around lines 89 - 102, getPresignedDownloadUrl currently uses the raw S3 key
(UUID_original) in the Content-Disposition; instead, store the original filename
as object metadata at upload (e.g., metadata key "original-filename" in your
uploadFile logic) and read that metadata here before building the presign. Call
HeadObjectRequest/HeadObjectResponse (via your s3 client) to fetch metadata
inside getPresignedDownloadUrl, fallback to the key if metadata is missing,
RFC5987-encode the filename (UTF-8''percent-encoded) and ensure any
quotes/newlines are sanitized, then set responseContentDisposition using
filename*=UTF-8''encodedName so downloads show the safe original filename when
you build the GetObjectRequest for s3Presigner.

---

41-53: Bucket verification swallows non-404 errors, then still attempts CORS configuration.

If headBucket fails with a non-404 S3Exception that's not rethrown into the outer catch (e.g., network error, auth error, region redirect), the outer catch (Exception e) logs and swallows, and execution falls through to the CORS block which will almost certainly also fail. Consider return-ing after a failed verify/create so CORS isn't attempted on an unverified bucket, and consider a health-indicator so operators can see the bucket wasn't provisioned rather than discovering it on the first user upload.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java`
around lines 41 - 53, The try/catch around s3Client.headBucket(...) swallows
non-404 S3Exception into the outer catch and lets execution continue to the CORS
configuration; update the FileService startup flow so that if headBucket fails
for any non-404 S3Exception (or any exception when verifying/creating the
bucket) you log the error and abort further bucket setup (e.g., return or throw
to stop executing the CORS/configuration block) instead of falling through, and
add or wire a health indicator/flag (e.g., bucketProvisioned boolean or health
check method) that is set only after successful s3Client.createBucket(...) or
headBucket verification so operators can detect an unprovisioned bucket.

src/main/java/org/example/visacasemanagementsystem/visa/mapper/VisaMapper.java (2)

59-61: Conditional set blocks clearing statusInformation via updateEntityFromDTO.

dto.statusInformation() != null means this mapper can never clear a previously set status message through the DTO; callers that want to clear it must do so out-of-band (as VisaService.updateVisa currently does at Line 248 via visa.setStatusInformation(null)). That's fine for the current flow, but be aware that any future caller expecting "null in DTO means clear" will silently keep the old value. If the intent is "only update when the DTO explicitly carries a value", that's okay — just flagging the semantic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/mapper/VisaMapper.java`
around lines 59 - 61, The current updateEntityFromDTO uses if
(dto.statusInformation() != null) to skip setting statusInformation when the DTO
value is null, which prevents callers from clearing an existing value; to fix,
either (A) always call visa.setStatusInformation(dto.statusInformation()) in
updateEntityFromDTO so a null in the DTO clears the field, or (B) add an
explicit presence indicator to the DTO (e.g., hasStatusInformation() or
Optional<String> statusInformation()) and update updateEntityFromDTO to set
visa.setStatusInformation(...) only when that presence flag indicates the caller
intended to update; update callers accordingly (e.g., VisaService.updateVisa) to
use the chosen approach.

---

15-35: toDTO() returns a VisaDTO with empty downloadUrls, which is then reconstructed in VisaService.findVisaDtoById — fragile split responsibility.

The mapper always sets downloadUrls to List.of() and findVisaDtoById then builds a brand-new VisaDTO copying every field just to replace downloadUrls with presigned URLs (see VisaService.findVisaDtoById Lines 61–89). This has two consequences:

1. Any other call site that uses visaMapper.toDTO(visa) directly (e.g., findAll, findVisasByApplicant, findVisaByType, findVisaByStatus, findVisaByDateCreated, etc.) silently returns visas with empty downloadUrls, which is a trap for future callers/templates.
2. The reconstruction in findVisaDtoById duplicates every field and will break silently if a new field is added to VisaDTO and forgotten there.

Consider either (a) passing a Function<String, String> s3KeyToUrl (or the FileService) into a dedicated mapper overload that populates downloadUrls in one place, or (b) using a wither-style helper like dto.withDownloadUrls(urls) so findVisaDtoById doesn't manually re-enumerate all fields.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/mapper/VisaMapper.java`
around lines 15 - 35, The mapper VisaMapper.toDTO currently sets downloadUrls to
an empty list causing callers to receive incomplete DTOs and forcing
VisaService.findVisaDtoById to reconstruct the entire VisaDTO; change the design
by either (A) adding an overload VisaMapper.toDTO(Visa visa,
Function<String,String> s3KeyToUrl) or toDTO(Visa visa, FileService fileService)
that maps visa.getS3Keys() -> presigned URLs and populates downloadUrls there,
or (B) add a withDownloadUrls(List<String>) method on VisaDTO (a wither) and
have VisaService.findVisaDtoById call visaMapper.toDTO(visa) and then
dto.withDownloadUrls(urls) instead of reconstructing the DTO; update all call
sites (findAll, findVisasByApplicant, findVisaByType, etc.) to use the
appropriate overload or wither so downloadUrls is consistently populated in one
place (refer to VisaMapper.toDTO, VisaService.findVisaDtoById, VisaDTO).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/main/resources/templates/visa/details.html (1)

186-194: substring(visa.s3Keys[iter.index], 37) is brittle — tightly coupled to FileService's key format.

The magic 37 encodes "UUID (36 chars) + _ (1 char)" from FileService.uploadFile. If that prefix ever changes (e.g., switch to a prefix path like visa/<id>/…, or a different id format), this throws StringIndexOutOfBoundsException at render time with no server-side signal. Safer options:

• Compute the display name in VisaService.findVisaDtoById alongside the presigned URL and expose a parallel fileNames list on VisaDTO.
• Or do the stripping in a shared helper (e.g., FileService.stripKeyPrefix(key)) so the 37 lives in one place with the key-generation code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/visa/details.html` around lines 186 - 194, The
template uses a brittle hardcoded substring call
(substring(visa.s3Keys[iter.index], 37)) tied to FileService.uploadFile key
format; move the filename extraction out of the view by either (a) computing and
attaching display names in VisaService.findVisaDtoById and adding a parallel
fileNames list on VisaDTO to render instead of slicing s3Keys, or (b) add a
single helper method in FileService (e.g., FileService.stripKeyPrefix(key)) that
encapsulates the prefix logic and call that from the service layer to populate
VisaDTO, then update the template to read the safe fileNames list (or sanitized
keys) instead of using substring on visa.s3Keys.

src/main/java/org/example/visacasemanagementsystem/file/FileService.java (1)

123-136: Presigned download filename leaks the UUID prefix.

responseContentDisposition uses fileName, which is the raw S3 key (<uuid>_<safeName>). Users who download the document will see e.g. a1b2c3…_passport.pdf as the save-as name. Since the key already has a fixed UUID + "_" prefix (37 chars), you can strip it here to present the original filename. If you ever want to show a true original name, consider persisting it alongside the key.

✏️ Suggested change

-        GetObjectRequest getObjectRequest = GetObjectRequest.builder()
-                .bucket(bucketName)
-                .key(fileName)
-                .responseContentDisposition("attachment; filename=\"" + fileName + "\"")
-                .build();
+        String displayName = fileName.length() > 37 ? fileName.substring(37) : fileName;
+        GetObjectRequest getObjectRequest = GetObjectRequest.builder()
+                .bucket(bucketName)
+                .key(fileName)
+                .responseContentDisposition("attachment; filename=\"" + displayName + "\"")
+                .build();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java`
around lines 123 - 136, The presigned download filename leaks the UUID prefix
because getPresignedDownloadUrl uses the raw S3 key (fileName) in
responseContentDisposition; update getPresignedDownloadUrl to derive a
displayName by stripping the fixed UUID + "_" prefix (e.g. split on the first
'_' or drop the first 37 chars) from fileName and use that displayName in
responseContentDisposition so users see the original filename; keep bucketName
and key as-is for the request, and consider persisting an originalName alongside
the key (e.g., in your upload method) if you need a canonical source for the
true original filename.

src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java (2)

61-89: findVisaDtoById destructures the DTO just to swap one field — consider a helper.

Rebuilding the 15-argument VisaDTO record by copying every field from the mapped result is noisy and is a new place to forget a field the next time VisaDTO grows. Two cleaner options:

• Add a small copy-helper on VisaDTO (e.g. withDownloadUrls(List<String>)) that returns a new record with only that field replaced.
• Or have VisaMapper.toDTO take an optional List<String> downloadUrls (default List.of()), and compute the URLs before calling the mapper.

♻️ Example (withDownloadUrls on the record)

// In VisaDTO
public VisaDTO withDownloadUrls(List<String> urls) {
    return new VisaDTO(id, visaType, visaStatus, nationality, passportNumber,
            travelDate, applicantId, applicantName, handlerId, handlerName,
            createdAt, updatedAt, statusInformation, urls, s3Keys);
}

-        VisaDTO dto = visaMapper.toDTO(visa);
-
-        List<String> presignedUrls = visa.getS3Keys().stream()
-                .map(fileService::getPresignedDownloadUrl)
-                .toList();
-
-        return new VisaDTO( dto.id(), /* …13 more fields… */ presignedUrls, dto.s3Keys());
+        List<String> presignedUrls = visa.getS3Keys().stream()
+                .map(fileService::getPresignedDownloadUrl)
+                .toList();
+        return visaMapper.toDTO(visa).withDownloadUrls(presignedUrls);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java`
around lines 61 - 89, The method findVisaDtoById currently reconstructs a 15-arg
VisaDTO to swap in presignedUrls; instead add a small copy-helper on the VisaDTO
record (e.g. public VisaDTO withDownloadUrls(List<String> urls)) that returns a
new VisaDTO with only the downloadUrls replaced, then change findVisaDtoById to
call visaMapper.toDTO(visa) and then
dto.withDownloadUrls(visa.getS3Keys().stream().map(fileService::getPresignedDownloadUrl).toList())
instead of rebuilding all fields; alternatively, if you prefer the mapper
approach, adjust visaMapper.toDTO to accept an optional List<String>
downloadUrls and pass the computed presignedUrls when calling it.

---

261-290: removeVisaDocument: the if (remove(s3Key)) is effectively dead, and orElseThrow() is opaque.

A couple of cleanups:

• Lines 274–276 already throw IllegalArgumentException when the key is not attached, so by the time execution reaches Line 278, remove(s3Key) always returns true. The outer if (and implicit silent no-op branch) can be dropped for clarity.
• Line 266: userRepository.findById(userId).orElseThrow() throws a bare NoSuchElementException with no message, which surfaces as a vague 500 if userId is stale. Prefer an EntityNotFoundException with a message, matching the pattern used elsewhere in this class.
• Consider also deleting from S3 after the in-memory remove and before visaRepository.save vs. its current position — current order (save → delete S3) means if fileService.deleteFile fails, the key is already gone from the DB and the audit log still records success. For a dangling orphan that's arguably better than a dangling DB reference, but worth being deliberate about.

♻️ Suggested simplification

-        User user = userRepository.findById(userId).orElseThrow();
+        User user = userRepository.findById(userId)
+                .orElseThrow(() -> new EntityNotFoundException("User not found."));
         boolean isOwner = visa.getApplicant().getId().equals(user.getId());
         boolean isAdmin = user.getUserAuthorization() != UserAuthorization.USER;

         if (!isOwner && !isAdmin) {
             throw new UnauthorizedException("You cannot delete this document.");
         }

         if (!visa.getS3Keys().contains(s3Key)) {
             throw new IllegalArgumentException("Document does not belong to this visa application.");
         }

-        if (visa.getS3Keys().remove(s3Key)) {
-            if (visa.getS3Keys().isEmpty()) {
-                visa.setVisaStatus(VisaStatus.INCOMPLETE);
-            } else {
-                visa.setVisaStatus(VisaStatus.SUBMITTED);
-            }
-            visaRepository.save(visa);
-
-            fileService.deleteFile(s3Key);
-
-            auditService.createAuditLog(userId, visaId, AuditEventType.UPDATED, "Removed document: " + s3Key);
-        }
+        visa.getS3Keys().remove(s3Key);
+        visa.setVisaStatus(visa.getS3Keys().isEmpty() ? VisaStatus.INCOMPLETE : VisaStatus.SUBMITTED);
+        visaRepository.save(visa);
+        fileService.deleteFile(s3Key);
+        auditService.createAuditLog(userId, visaId, AuditEventType.UPDATED, "Removed document: " + s3Key);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java`
around lines 261 - 290, In removeVisaDocument, simplify logic by removing the
redundant if around visa.getS3Keys().remove(s3Key) (since you already validate
membership), replace userRepository.findById(userId).orElseThrow() with a clear
EntityNotFoundException including the userId (e.g., throw new
EntityNotFoundException("User not found: " + userId)) and change the operation
order so you remove the s3Key from visa.getS3Keys(), call
fileService.deleteFile(s3Key) immediately after the in-memory removal (so
deletion happens before persisting), then update visa status, call
visaRepository.save(visa), and finally call auditService.createAuditLog(userId,
visaId, AuditEventType.UPDATED, "Removed document: " + s3Key).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/main/java/org/example/visacasemanagementsystem/file/FileService.java (3)

129-142: Presigned download filename includes the UUID prefix.

responseContentDisposition is set to attachment; filename="<UUID>_<sanitized-original>", so end users downloading a passport document will see the internal UUID in the saved filename. Consider storing the original (sanitized) filename alongside the S3 key (e.g., as object metadata or in the DB) and passing only the user-friendly portion to responseContentDisposition while keeping the UUID in the S3 key for uniqueness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java`
around lines 129 - 142, getPresignedDownloadUrl currently sets
responseContentDisposition to the full S3 key (which includes the internal UUID)
so users download filenames like "<UUID>_original"; instead, change
getPresignedDownloadUrl to look up the stored original/sanitized filename (from
object metadata or your DB) and pass only that user-friendly name to
GetObjectRequest.responseContentDisposition while keeping the UUID-prefixed S3
key for bucketName/key; update the method to fetch the original filename (via
your metadata lookup), set responseContentDisposition to "attachment;
filename=\"<originalFilename>\"" and continue using s3Presigner.presignGetObject
with the existing GetObjectPresignRequest.

---

97-108: Prefer a validation-specific exception over IOException for size/type rejections.

Throwing IOException for user-input validation failures (size exceeded, disallowed content type) conflates client errors with genuine I/O failures. Upstream (VisaViewController) will likely translate this into a 500, whereas these are really 400 Bad Request conditions. Consider IllegalArgumentException or a dedicated InvalidUploadException so the web layer can map it to a proper status and user-friendly message.

♻️ Proposed refactor

-    public String uploadFile(MultipartFile file) throws IOException {
-        // Validate file size
-        if (file.getSize() > MAX_FILE_SIZE_BYTES) {
-            throw  new IOException("File exceeds maximum allowed size of 10 MB");
-        }
-
-        // Validate content type
-        String contentType = file.getContentType();
-        if (contentType == null || !ALLOWED_CONTENT_TYPES.contains(contentType)) {
-            throw new IOException("Unsupported document type: " + contentType
-                    + ". Allowed: PDF, JPEG, PNG");
-        }
+    public String uploadFile(MultipartFile file) throws IOException {
+        // Validate file size
+        if (file.getSize() > MAX_FILE_SIZE_BYTES) {
+            throw new IllegalArgumentException("File exceeds maximum allowed size of 10 MB");
+        }
+
+        // Validate content type
+        String contentType = file.getContentType();
+        if (contentType == null || !ALLOWED_CONTENT_TYPES.contains(contentType)) {
+            throw new IllegalArgumentException("Unsupported document type: " + contentType
+                    + ". Allowed: PDF, JPEG, PNG");
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java`
around lines 97 - 108, Replace the use of IOException for validation failures in
FileService.uploadFile with a validation-specific unchecked exception (create a
new InvalidUploadException extends RuntimeException) so client errors map to
400; keep IOException for real I/O problems in the method signature, but throw
new InvalidUploadException(...) instead of new IOException(...) for the size
check and the content-type check (references: uploadFile method in FileService
and the two throw sites that validate MAX_FILE_SIZE_BYTES and
ALLOWED_CONTENT_TYPES); add the new exception class and import it, and ensure
VisaViewController (or the web layer) can map InvalidUploadException to HTTP
400.

---

50-50: Hardcoded "test-bucket" sentinel for skipping initialization.

Relying on a magic bucket-name string to detect the test profile is brittle — any production bucket accidentally named test-bucket would silently skip initialization, and vice versa. Prefer a Spring profile check (@Profile/@ConditionalOnProperty) or an explicit flag like minio.initializeBucket=false in application-test.properties.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java` at
line 50, The FileService currently uses a hardcoded sentinel check (bucketName
== null || bucketName.equals("test-bucket")) to skip bucket initialization;
replace that brittle logic with a proper profile/property-driven guard: remove
the "test-bucket" string check in the initialization path (the code referencing
bucketName in FileService) and instead inject/consume a Spring boolean property
(e.g., minio.initializeBucket) or annotate the bean with
`@Profile/`@ConditionalOnProperty so initialization only runs when explicitly
enabled; update the initialization method/constructor (initBucket or whatever
method performs creation) to consult this property/annotation and fall back to
checking only for null bucketName, ensuring production buckets named
"test-bucket" are not treated specially.